Skip to content

[rb] Update selenium manager types#14189

Merged
p0deje merged 12 commits intoSeleniumHQ:trunkfrom
aguspe:rb_leverage_streaming_output
Jul 9, 2024
Merged

[rb] Update selenium manager types#14189
p0deje merged 12 commits intoSeleniumHQ:trunkfrom
aguspe:rb_leverage_streaming_output

Conversation

@aguspe
Copy link
Contributor

@aguspe aguspe commented Jun 25, 2024 •
edited
Loading

User description

Description

This PR adds the correct type after the Selenium manager class has been updated for ruby

Motivation and Context

It's important as described here #13989 that we utilize the new options available for us in the Selenium manager implementation and that we update the respective types

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement

Description

  • Updated the Selenium manager commands in the Ruby client to use mixed output format instead of JSON.
  • Changed the debug flag to use --log-level debug for better logging control.

Changes walkthrough 📝

Relevant files
Enhancement
selenium_manager.rb
Update Selenium manager command options for Ruby client   

rb/lib/selenium/webdriver/common/selenium_manager.rb

  • Changed command output format from JSON to mixed.
  • Updated debug flag to log-level debug.
    		•  +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 25, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Output Format Change:
    Ensure that the change from JSON to mixed output format in the Selenium manager does not break existing integrations or data parsing.
    Logging Level:
    Verify that the new --log-level debug flag integrates smoothly with the existing logging system and does not introduce excessive verbosity that could obscure important log messages.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 25, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use a single array concatenation operation to add multiple command options at once

    Consider using a single array concatenation operation to add all the new command options
    at once. This can make the code more concise and potentially improve performance slightly.

    rb/lib/selenium/webdriver/common/selenium_manager.rb [115-117]

    -command += %w[--language-binding ruby]
-command += %w[--output mixed]
+command += %w[--language-binding ruby --output mixed]
 command << '--log-level debug' if WebDriver.logger.debug?
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to combine array concatenations into a single operation is valid for improving code conciseness and slightly enhancing performance.

    		7
    Possible issue
    Verify the compatibility of the --log-level debug option with the selenium manager

    Ensure that the --log-level debug option is compatible with the rest of the command
    options and the expected behavior of the selenium manager, as changing from --debug to
    --log-level debug might have different effects.

    rb/lib/selenium/webdriver/common/selenium_manager.rb [117]

    -command << '--log-level debug' if WebDriver.logger.debug?
+command << '--log-level debug' if WebDriver.logger.debug? # Ensure compatibility with selenium manager
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: This is a prudent suggestion to ensure that changes in command line options do not unintentionally alter the behavior of the system, although it lacks specific details on how to verify compatibility.

    		6

    @aguspe aguspe changed the title [rb] Start working on updating the ruby client to leverage the new selenium manager implementation [rb] Update selenium manager types Jun 28, 2024
    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jun 28, 2024

    @titusfortner I saw the update you did on the selenium manager class, so I changed my PR to add the right types and update the correspondent RBS file for the selenium manager class

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 5, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    @p0deje
    Copy link
    Member

    p0deje commented Jul 7, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    Hi, sorry for delay. Can you please take a look at the CI failures?

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 7, 2024

    Hello @p0deje I hope you are having a great evening, whenever you have time could you review this PR? thank you for all your help

    Hi, sorry for delay. Can you please take a look at the CI failures?

    Thank you so for taking the time, and no problem I know you are busy.

    Now I remove the last updates on arguments so this PR just updates the types after the changes done to the selenium manager class

    @p0deje
    Copy link
    Member

    p0deje commented Jul 9, 2024

    @aguspe Can you please take a look at linter failures? https://github.com/SeleniumHQ/selenium/actions/runs/9828809475/job/27218278086?pr=14189

    @aguspe
    Copy link
    Contributor Author

    aguspe commented Jul 9, 2024

    @p0deje the linter issues are fixed now and only the RBS file is left

    @p0deje p0deje merged commit df75c3a into SeleniumHQ:trunk Jul 9, 2024
    @p0deje
    Copy link
    Member

    p0deje commented Jul 9, 2024

    Thank you!

    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @aguspe @sandeepsuryaprasad
    [rb] Update selenium manager types (SeleniumHQ#14189)
    488df1e 
    Co-authored-by: aguspe <agustin.pe94@gmail.com>
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Nov 8, 2024
    Open
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @aguspe @M1troll
    [rb] Update selenium manager types (SeleniumHQ#14189)
    4bbfbd4 
    Co-authored-by: aguspe <agustin.pe94@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @aguspe @p0deje